Skip to content

Conversation

@darionyaphet
Copy link
Contributor

STORM-1875

Separate Jedis / JedisCluster to provide full operations for each environment to users .

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jun 3, 2016

The problem is actually not simple. The blocking point is JedisCommands interface, not config.
JedisCommands exposes operations which use only single key. Since some multiple keys operations of Redis is not suitable for Redis Cluster (operation is available but not making sense), Jedis and JedisCluster takes different interfaces, which blocks us to provide unified interface from storm-redis side.

One simple way is separating existing classes to RedisLookupBolt / RedisClusterLookupBolt, RedisStoreBolt / RedisClusterStoreBolt, as State/MapState did, but it incurs backward incompatibility if users use RedisLookupBolt / RedisStoreBolt to access Redis Cluster.

@HeartSaVioR
Copy link
Contributor

One other way without breaking compatibility but not beauty is adding some utility methods to AbstractRedisBolt like,

public JedisCluster asJedisCluster(JedisCommands instance) throws IllegalArgumentException
public JedisCluster asJedis(JedisCommands instance) throws IllegalArgumentException

and let users convert their JedisCommands instance to actual type before using full features.

@darionyaphet
Copy link
Contributor Author

darionyaphet commented Jun 5, 2016

Both Jedis and JedisCluster have implemented JedisCommands interface . So I think we can use JedisCommands calling the same method on single redis instance and redis cluster .

Just use JedisConfig to control instance JedisContainer or JedisCommandsInstanceContainer maybe a good idea and support two difference interface is useless :P

(We have support some command like SET , RPUSH , HSET etc . I don't found which command is not support ? )

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jun 5, 2016

@darionyaphet
This is a part of Jedis class diagram when Jedis was 2.5.0. At that time JedisCluster doesn't support multiple keys operation so now it becomes more complicated.
https://lh5.googleusercontent.com/-kMPsNHe6t2I/U0Dn01HxIoI/AAAAAAAAt4U/U8I0j570rI4/s3200/Jedis+Class+Diagram+without+fields_methods.jpg

DISCLAIMER: I'm also a committer of Jedis. :)

@darionyaphet
Copy link
Contributor Author

Storm-Redis have support multiple keys operation ?

@HeartSaVioR
Copy link
Contributor

No for predefined operations (StoreBolt, LookupBolt) but we can still support full features for AbstractRedisBolt and let users extend this class.

@darionyaphet
Copy link
Contributor Author

OK , I will try to separating classes :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants